✨ Make bootMACAddress optional for virtual media with inspection enabled #2793

donpenney · 2025-11-17T19:22:57Z

Virtual media BMC drivers (redfish-virtualmedia, idrac-virtualmedia,
ilo5-virtualmedia) can boot from virtual media and discover the MAC
address during hardware inspection. Therefore, bootMACAddress is optional
when inspection is enabled but still required when inspection is disabled
(since there's no other way to discover the MAC address).

This change updates the virtual media driver implementations to return
false from their NeedsMAC() methods, and adds validation logic to require
bootMACAddress when virtual media is used with inspection disabled.

Drivers that require PXE boot (like libvirt, ipmi) continue to require
bootMACAddress in all cases.

Changes:

pkg/hardwareutils/bmc/redfish_virtualmedia.go: NeedsMAC() returns false
pkg/hardwareutils/bmc/idrac_virtualmedia.go: NeedsMAC() returns false
internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go:
Add logic to require bootMACAddress for virtual media when inspection
is disabled (via InspectionMode field or inspect.metal3.io annotation)
pkg/provisioner/ironic/ironic.go: Skip MAC-based port queries when
bootMACAddress is empty to prevent false MAC conflicts

Test Coverage:

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go:
- bootMACAddress not required for virtual media with inspection enabled
- bootMACAddress required for virtual media with inspection disabled
- bootMACAddress valid when provided for virtual media with inspection disabled
pkg/hardwareutils/bmc/access_test.go:
Update virtual media test expectations (needsMac: false)

Assisted-By: Claude [email protected]

Checklist:

Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
E2E tests have been added, if necessary.
Integration tests have been added, if necessary.

metal3-io-bot · 2025-11-17T19:23:09Z

Hi @donpenney. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dtantsur · 2025-11-18T12:31:12Z

I'm not sure I understand the link between PreprovisioningNetworkDataName and BootMacAddress. Honestly, I suggest just dropping the requirement on BootMacAddress for virtual media based drivers.

donpenney · 2025-11-18T13:38:03Z

I'm not sure I understand the link between PreprovisioningNetworkDataName and BootMacAddress. Honestly, I suggest just dropping the requirement on BootMacAddress for virtual media based drivers.

Thanks @dtantsur . I was aiming to be a bit surgical here to minimize behavoural changes. My thinking was that if the network config has been provided in an nmstate secret as part of the initial configuration, then the BootMacAddress was unnecessary for the inspection stage. I assumed it would always be the case for inspection with virtual media, but hadn't thought about using that as the determining factor, so I'll go that route instead.

dtantsur · 2025-11-18T13:54:11Z

For background: since quite some time, the Ironic inspection implementation for Redfish is actually hybrid: it starts with getting MAC addresses from Redfish, then reboots into the agent ramdisk.

Now that I said that, I think we should still require BootMacAddress if inspection is disabled on the BMH @donpenney.

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

dtantsur · 2025-11-19T15:40:00Z

/ok-to-test

elfosardo · 2025-11-20T10:25:14Z

/lgtm

Virtual media BMC drivers (redfish-virtualmedia, idrac-virtualmedia, ilo5-virtualmedia) can boot from virtual media and discover the MAC address during hardware inspection. Therefore, bootMACAddress is optional when inspection is enabled but still required when inspection is disabled (since there's no other way to discover the MAC address). This change updates the virtual media driver implementations to return false from their NeedsMAC() methods, and adds validation logic to require bootMACAddress when virtual media is used with inspection disabled. Drivers that require PXE boot (like libvirt, ipmi) continue to require bootMACAddress in all cases. Changes: - pkg/hardwareutils/bmc/redfish_virtualmedia.go: NeedsMAC() returns false - pkg/hardwareutils/bmc/idrac_virtualmedia.go: NeedsMAC() returns false - internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go: Add logic to require bootMACAddress for virtual media when inspection is disabled (via InspectionMode field or inspect.metal3.io annotation) - pkg/provisioner/ironic/ironic.go: Skip MAC-based port queries when bootMACAddress is empty to prevent false MAC conflicts Test Coverage: - internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go: * bootMACAddress not required for virtual media with inspection enabled * bootMACAddress required for virtual media with inspection disabled * bootMACAddress valid when provided for virtual media with inspection disabled - pkg/hardwareutils/bmc/access_test.go: Update virtual media test expectations (needsMac: false) Assisted-By: Claude <[email protected]> Signed-off-by: Don Penney <[email protected]>

Changes: 1. Restore case-insensitive MAC comparison using strings.EqualFold() - The EqualFold check was removed inadvertently but is necessary for case-insensitive bootMACAddress validation 2. Use existing host.InspectionDisabled() method - Remove duplicate isInspectionDisabled() function - Replace custom implementation with the existing BareMetalHost method - The host.InspectionDisabled() method provides identical functionality Assisted-by: Claude (AI Assistant) Signed-off-by: Don Penney <[email protected]>

donpenney · 2025-11-28T16:02:14Z

Rebased my branch and removed a change to a test case that had been failing in my env due to running with golang 1.24.5. The format of an url.Parse error message changed in 1.24.9. Because my golang was older, the test was failing in my environment. I have now removed this unrelated change from my update

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

dtantsur · 2025-11-29T13:39:33Z

/approve

metal3-io-bot · 2025-11-29T13:39:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dtantsur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Simplify the conditional logic for determining when a MAC address is required. The new approach uses a single boolean expression instead of separate assignment and conditional check, improving code readability while maintaining the same functionality. Assisted-by: Claude (AI Assistant) Signed-off-by: Don Penney <[email protected]>

Copilot

Pull request overview

This PR makes the bootMACAddress field optional for virtual media BMC drivers (redfish-virtualmedia, idrac-virtualmedia, ilo5-virtualmedia) when hardware inspection is enabled, while still requiring it when inspection is disabled or for PXE-based drivers that always need the MAC address.

Key Changes:

Virtual media drivers can now boot and discover the MAC address during inspection, eliminating the requirement to pre-configure bootMACAddress
PXE-based drivers (libvirt, ipmi) continue to require bootMACAddress in all cases
Enhanced validation logic properly handles the conditional requirement based on driver type and inspection status

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/hardwareutils/bmc/redfish_virtualmedia.go	Updated `NeedsMAC()` to return false for redfish-virtualmedia and ilo5-virtualmedia drivers
pkg/hardwareutils/bmc/idrac_virtualmedia.go	Updated `NeedsMAC()` to return false for idrac-virtualmedia driver
pkg/hardwareutils/bmc/access_test.go	Updated test expectations to reflect that virtual media drivers no longer need MAC by default
internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go	Added conditional MAC requirement logic: requires MAC when driver needs it OR when inspection is disabled
internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go	Added test coverage for virtual media with/without inspection, added blank import for BMC driver registration
internal/webhooks/metal3.io/v1alpha1/baremetalhost_webhook_test.go	Added test case for libvirt driver requiring MAC address
pkg/provisioner/ironic/ironic.go	Skips MAC-based port lookup when bootMACAddress is empty to prevent false conflict detection
pkg/provisioner/ironic/findhost_test.go	Added tests verifying no port queries occur when MAC is empty

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

elfosardo · 2025-12-01T12:07:38Z

/lgtm

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 17, 2025

metal3-io-bot requested review from honza and zaneb November 17, 2025 19:23

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 17, 2025

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 17, 2025

donpenney force-pushed the allow-unset-boot-mac branch from dcd4003 to da22bd4 Compare November 18, 2025 22:02

donpenney changed the title ~~✨ Allow BMH registration without bootMACAddress when preprovisioningNet…~~ ✨ Make bootMACAddress optional for virtual media with inspection enabled Nov 18, 2025

donpenney marked this pull request as ready for review November 19, 2025 14:51

metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 19, 2025

metal3-io-bot requested review from dtantsur and lentzi90 November 19, 2025 14:51

donpenney mentioned this pull request Nov 19, 2025

Add support for deferred boot MAC population from interface labels openshift-kni/oran-o2ims#1427

Merged

dtantsur reviewed Nov 19, 2025

View reviewed changes

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go Outdated Show resolved Hide resolved

dtantsur reviewed Nov 19, 2025

View reviewed changes

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go Outdated Show resolved Hide resolved

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 19, 2025

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2025

donpenney added 2 commits November 28, 2025 10:58

donpenney force-pushed the allow-unset-boot-mac branch from d3d109c to a827cd6 Compare November 28, 2025 15:59

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2025

dtantsur reviewed Nov 29, 2025

View reviewed changes

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go Outdated Show resolved Hide resolved

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2025

tuminoid requested a review from Copilot November 30, 2025 19:29

Copilot started reviewing on behalf of tuminoid November 30, 2025 19:29 View session

Copilot finished reviewing on behalf of tuminoid November 30, 2025 19:33

Copilot AI reviewed Nov 30, 2025

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2025

metal3-io-bot merged commit 346570b into metal3-io:main Dec 1, 2025
24 checks passed

metal3-io-bot added this to the BMO - v0.12 milestone Dec 1, 2025

✨ Make bootMACAddress optional for virtual media with inspection enabled #2793

✨ Make bootMACAddress optional for virtual media with inspection enabled #2793

Uh oh!

Conversation

donpenney commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metal3-io-bot commented Nov 17, 2025

Uh oh!

dtantsur commented Nov 18, 2025

Uh oh!

donpenney commented Nov 18, 2025

Uh oh!

dtantsur commented Nov 18, 2025

Uh oh!

Uh oh!

Uh oh!

dtantsur commented Nov 19, 2025

Uh oh!

elfosardo commented Nov 20, 2025

Uh oh!

donpenney commented Nov 28, 2025

Uh oh!

Uh oh!

dtantsur commented Nov 29, 2025

Uh oh!

metal3-io-bot commented Nov 29, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

elfosardo commented Dec 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

donpenney commented Nov 17, 2025 •

edited

Loading